Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

fix: honor explicit prefix #254

Merged
merged 2 commits into from
Mar 23, 2021
Merged

fix: honor explicit prefix #254

merged 2 commits into from
Mar 23, 2021

Conversation

jameschensmith
Copy link
Contributor

@jameschensmith jameschensmith commented Mar 17, 2021

Summary

Honors explicit prefix. npm-package-arg was being used incorrectly in reify when saving the ideal tree. Only the spec was being passed in, which resulted in some unusual results (submitted a ticket to address that in npm-package-arg as well).

Once this was addressed, another issue was spotted, where passing no version on a new package results in * being added. Previously, the type resolved to tag. This now resolves correctly to range. When resolving the variable range shortly after, though, existing checks are now truthy. Before, it was short-circuited as falsey due to type. Also, node-semver returns false when comparing subsets of * other than itself (it's documented in the code). The now falsey resolution makes spec(*) used in a user's package.json rather than prefixRange.

This request addresses the false semver subset check by checking if spec is *. * should still resolve in a package.json if child.version is falsy for any reason.

In addition, there was a test which had an unusual alias. This fix brought that abnormality to light. This request corrects that test.

References

Fixes #253.

@@ -905,7 +905,7 @@ module.exports = cls => class Reifier extends cls {
// would allow versions outside the requested range. Tags and
// specific versions save with the save-prefix.
const isRange = (subSpec || req).type === 'range'
const range = !isRange || subset(prefixRange, spec, { loose: true })
const range = !isRange || subset(prefixRange, spec, { loose: true }) || spec === '*'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the route to go, thoughts on switching these checks for speed?

Suggested change
const range = !isRange || subset(prefixRange, spec, { loose: true }) || spec === '*'
const range = !isRange || spec === '*' || subset(prefixRange, spec, { loose: true })

@jameschensmith
Copy link
Contributor Author

jameschensmith commented Mar 18, 2021

Before merging this PR, I would love for the subset fix to be resolved in node-semver. This would remove the || spec === '*' logic. I created an issue for it (npm/node-semver#374), and will be submitting a PR shortly. The PR is now up (npm/node-semver#375).

@@ -808,7 +808,7 @@ t.test('saving the ideal tree', t => {
dependencies: {
a: 'git+ssh://git@github.com:foo/bar#baz',
b: '',
d: 'd@npm:c@1.x <1.9.9',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks weird. Probably shouldn't end up with the name in there...

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I don't think this is quite the right way to go. I might be wrong, and I agree that there is a bug here, but it'd be much easier to evaluate if we start from a failing test (such as the case you suggested), and then figure out exactly where it's going wrong and how to make that test pass.

I'll dig in today and try to get more insight. At the very least, saving the name on the right-hand-side of the dependencies field is definitely a bug, that shouldn't be happening.

@isaacs
Copy link
Contributor

isaacs commented Mar 23, 2021

Ah, ok, the || spec === '*' part is unnecessary, but now that npm/node-semver#377 has shipped, I think the other bit looks correct.

This patch seems like it fixes the problem, and is pretty close to what you put here. Want to take a look? https://gist.github.com/isaacs/7d42bb2672905d3b27e70bc4015ca135

@jameschensmith
Copy link
Contributor Author

jameschensmith commented Mar 23, 2021

Ah, ok, the || spec === '*' part is unnecessary, but now that npm/node-semver#377 has shipped, I think the other bit looks correct.

This patch seems like it fixes the problem, and is pretty close to what you put here. Want to take a look? gist.github.com/isaacs/7d42bb2672905d3b27e70bc4015ca135

Yes! That's what I was planning to do after the patch in node-semver (i.e. remove the || spec === '*' and bump semver; the additions to the test apart from what I did are great as well!). 😁👏 If you want me to finish the PR up, let me know. Otherwise, let's get this patched! 🙌

@jameschensmith
Copy link
Contributor Author

Went ahead and addressed the requested changes. Very straightforward 😊 Thanks for your reviews today!

James Chen-Smith added 2 commits March 23, 2021 09:06
Honors explicit prefix.

Signed-off-by: James Chen-Smith <jameschensmith@gmail.com>

PR-URL: #254
Credit: @jameschensmith
Close: #254
Reviewed-by: @isaacs
@isaacs isaacs closed this in 5a4b58c Mar 23, 2021
@isaacs isaacs merged commit 5a4b58c into npm:main Mar 23, 2021
@isaacs
Copy link
Contributor

isaacs commented Mar 23, 2021

This is shipped now! Will be in next npm release. I shuffled around the commit history a bit so that semver update is in one, and the functional updates come together after it. Thanks!

@jameschensmith jameschensmith deleted the jameschensmith/honor-explicit-prefix branch March 23, 2021 22:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] (CLI-related) npm install ignores explicit prefix
2 participants